Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[17.0][MIG] hr_contract_currency #1357

Open
wants to merge 5 commits into
base: 17.0
Choose a base branch
from
Open

[17.0][MIG] hr_contract_currency #1357

wants to merge 5 commits into from

Conversation

baptiste-n42
Copy link

@baptiste-n42 baptiste-n42 commented Jun 4, 2024 •
edited
Loading

Migration to v17:

Work done :

  • Update manifest version
  • Add thai localization
  • Update view form
  • Add warning if using this module with Odoo accounting enterprise enabled
  • Add the custom currency to Odoo Payslips (If hr_payroll is installed)

@baptiste-n42 baptiste-n42 mentioned this pull request Jun 4, 2024
Open
22 tasks
@baptiste-n42 baptiste-n42 force-pushed the 17.0-mig-hr_contract_currency branch 2 times, most recently from 0e31631 to 2c4303d Compare June 4, 2024 06:28
@baptiste-n42 baptiste-n42 force-pushed the 17.0-mig-hr_contract_currency branch from 2c4303d to e4c3869 Compare June 4, 2024 06:34
@baptiste-n42 baptiste-n42 marked this pull request as draft June 5, 2024 02:38
@baptiste-n42 baptiste-n42 marked this pull request as ready for review June 5, 2024 04:13
@baptiste-n42 baptiste-n42 force-pushed the 17.0-mig-hr_contract_currency branch from c6396fe to 1f07a7a Compare June 10, 2024 04:33
@baptiste-n42
Revert "[ADD] Display the selected currency on the final Payslip if O…
e4ea956 
…doo enterprise is installed"

- Modules from Odoo enterpri break the .

This reverts commit 1274365.
@baptiste-n42 baptiste-n42 force-pushed the 17.0-mig-hr_contract_currency branch 2 times, most recently from e312381 to 17435c3 Compare June 10, 2024 07:15
@baptiste-n42
[FIX] Corrected currency assignment for multi-company instances with …
cfbcd57 
…varying currencies.

Previously, Odoo defaulted to the currency of the first company when updating existing records.
This issue has been addressed by utilizing a computed field.

The `currency_id` field was being cut off at a 120% zoom level.
By assigning the field to its own group, this display issue has been resolved without impacting user experience.
@baptiste-n42 baptiste-n42 force-pushed the 17.0-mig-hr_contract_currency branch from 17435c3 to cfbcd57 Compare June 10, 2024 07:22
@baptiste-n42
Copy link
Author

@peluko00 @mpascuall Can you please review ?

mpascuall
mpascuall approved these changes Jul 17, 2024
View reviewed changes
Copy link

@mpascuall mpascuall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested in runboat

peluko00
peluko00 approved these changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@peluko00 peluko00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested in runboat

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

pleirb
pleirb approved these changes Jul 31, 2024
View reviewed changes
Copy link

@pleirb pleirb left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested in runboat. Is it planned to port a version 16 as well?

@pleirb pleirb mentioned this pull request Jul 31, 2024
Open
@baptiste-n42
Copy link
Author

LGTM, tested in runboat. Is it planned to port a version 16 as well?

I think the migration to v16 could be very easy since the module is fairly small and not that complex.

Do we have a timeline for merging the PR in the stable branch ?

@pleirb
Copy link

pleirb commented Aug 20, 2024

LGTM, tested in runboat. Is it planned to port a version 16 as well?

I think the migration to v16 could be very easy since the module is fairly small and not that complex.

Do we have a timeline for merging the PR in the stable branch ?

Yes, that's what I did, I ported this version for v16 but ran into a lot of difficulties with several dependent modules as a comment in issue #167.
How did you resolve the multiple currency issue so you could post accounting entries correctly in version 17?

@baptiste-n42
Copy link
Author

baptiste-n42 commented Aug 21, 2024 •
edited
Loading

How did you resolve the multiple currency issue so you could post accounting entries correctly in version 17?

I didn't.. This module is only for reference, if you're posting in a journal you will use the journal's currency. I don't think you can have a multi-currency journal system, Odoo will just convert foreign currency to the journal currency based on the latest rate.

@baptiste-n42
Copy link
Author

@peluko00 @mpascuall how can I add this module to issue #1291 modules list ?

@mpascuall
Copy link

@peluko00 @mpascuall how can I add this module to issue #1291 modules list ?

Hi! You can check this link that contains the information you need: https://github.com/OCA/maintainer-tools/wiki/%5BFAQ%5D-Missing-modules-in-migration-issue-list

@baptiste-n42
Copy link
Author

Hi! You can check this link that contains the information you need: https://github.com/OCA/maintainer-tools/wiki/%5BFAQ%5D-Missing-modules-in-migration-issue-list

Hi, yes I read it. So do you need to add this module to the list for this PR to be merged ?

@peluko00
Copy link
Contributor

Hi! You can check this link that contains the information you need: https://github.com/OCA/maintainer-tools/wiki/%5BFAQ%5D-Missing-modules-in-migration-issue-list

Hi, yes I read it. So do you need to add this module to the list for this PR to be merged ?

The PSC will add it when review it, don't worry abou that.

dreispt
dreispt requested changes Dec 10, 2024
View reviewed changes
Copy link
Member

@dreispt dreispt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the commit history, I miss the pre 17.0 commits, and I see por 17.0 fix commits, that shouldn't be new in a migration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreispt dreispt dreispt requested changes

@mpascuall mpascuall mpascuall approved these changes

@peluko00 peluko00 peluko00 approved these changes

@pleirb pleirb pleirb approved these changes

Assignees
No one assigned
Labels
needs fixing needs more information
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@baptiste-n42 @OCA-git-bot @pleirb @mpascuall @peluko00 @dreispt